refactor(concurrent): typed run plan + recorded estimand/population metadata (#394 pillar 4) - #449
Open
ethanbuckley wants to merge 1 commit into
Open
refactor(concurrent): typed run plan + recorded estimand/population metadata (#394 pillar 4)#449ethanbuckley wants to merge 1 commit into
ethanbuckley wants to merge 1 commit into
Conversation
…etadata (#394 pillar 4) Extends the ITT / gain-factor / level-factor / DiD run-plan pattern to the concurrent conditional-associations family. A new concurrent.py provides ConcurrentModelSettings (fail-fast on a misspelled spec.extra key; validates the 5 legacy keys) and a resolved ConcurrentRunPlan that drives data preparation and the config.json / model_recipe.md audit trail, recording the design, estimand, causal status (associational only -- the Table-2 fallacy applies), analysis population and missing-data assumption alongside every fit. fit_concurrent now resolves and validates the plan before the context resets an output directory or the loader reads data, then loads via plan.prepare_kwargs() (levels frame; outcomes = outcome + predictor skills, de-duplicated); the per-wave factory calls stay in the pipeline using plan attributes because the model is fit once per wave with a wave-specific usable-predictor subset and varying age/group. predictor_slope_sigma is None on the plan when unset, so the pipeline fills the build_concurrent_model default via _default_of (the anti-drift single source #394 retains). reporting._resolved_run_plan gains a concurrent branch so write_model_recipe and config.json's resolved_run_plan pick it up. Byte-identical: the resolved prepare kwargs, settings and slope-prior sigma reproduce the former inline fit_concurrent logic exactly for all 11 registered concurrent models (verified programmatically). ca-001 dev fit is clean end-to-end and now emits model_recipe.md + a populated resolved_run_plan. 14 new run-plan tests; drift and concurrent-characterisation suites pass; ruff / format:check / spellcheck clean. Part of #394 (pillar 4: typed settings + resolved plans). No estimand, likelihood, prior, population, fitted equation, sampling preset or artefact schema changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the statistical pipeline’s “typed settings + resolved run plan” boundary to the concurrent conditional-associations family, making the family’s configuration fail-fast (rejecting unknown spec.extra keys) and ensuring resolved estimand/design/population metadata is recorded consistently in config.json and model_recipe.md, in line with issue #394’s pillar-4 requirements.
Changes:
- Add
ConcurrentModelSettingsandConcurrentRunPlanplusresolve_concurrent_run_plan()to validate/resolve concurrent model configuration before any data are loaded. - Update
fit_concurrentto consume the resolved plan for preparation/metadata, while keeping per-wave factory calls in the pipeline. - Extend reporting metadata plumbing and add a new test suite covering the concurrent run-plan contract and registered-spec resolution.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/language_reading_predictors/statistical_models/concurrent.py |
Introduces typed concurrent settings + validated resolved run plan with recorded design/estimand/population metadata and preparation kwargs. |
src/language_reading_predictors/statistical_models/pipeline.py |
Refactors fit_concurrent to resolve/store the plan early, drive load_and_prepare from plan kwargs, and preserve anti-drift defaulting via _default_of. |
src/language_reading_predictors/statistical_models/reporting.py |
Adds a concurrent run-plan branch so resolved_run_plan + model_recipe.md are emitted for the family. |
tests/statistical_models/test_concurrent_run_plan.py |
Adds validation, resolution, and registered-spec coverage tests for the concurrent family’s typed run-plan. |
Comment on lines
2288
to
2290
| def _resolved_run_plan(context: StatisticalFitContext): | ||
| """The typed run plan for families that have one (ITT, gain factors), else None.""" | ||
| """The typed run plan for families that have one (ITT, gain, concurrent), else None.""" | ||
| if context.spec.kind == "itt": |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
Drafted by an LLM-based AI tool (Claude Code/Opus 4.8).
#394 pillar 4 (typed settings + resolved plans), extending the pattern from the merged ITT/gain-factor (#418) and the level-factor (#445) + DiD (#448) run-plans to the concurrent conditional-associations family. Independent, off
main.What this does
A new
concurrent.pygives the family the same typed boundary:ConcurrentModelSettings— immutable, validated;from_legacy_extrarejects unknownspec.extrakeys (a misspelledpredictor_symbolfails before data loading), validates the 5 legacy keys.predictor_slope_sigmadefaults toNoneso the pipeline fills thebuild_concurrent_modeldefault via_default_ofreflection — keeping the anti-drift single source Complete the statistical pipeline family split and artefact lifecycle refactor #394 asks to retain, not a duplicated literal.ConcurrentRunPlan— resolved and validated before the context resets an output directory or the loader reads data; drivesload_and_prepareand records the design / estimand / causal status / analysis population / missing-data assumption. The estimand prose states plainly that this family is associational only (the Table-2 fallacy applies — conditioning on contemporaneous post-treatment skill levels is intentional).fit_concurrentconsumesplan.prepare_kwargs()(levels frame; outcomes = outcome + predictor skills, de-duplicated). The per-wave factory calls stay in the pipeline using the plan attributes, because the model is fit once per wave with a wave-specific usable-predictor subset and the bivariate refits varyinclude_age/include_group— so a singlefactory_kwargsdoesn't fit the family.reporting._resolved_run_plangains aconcurrentbranch so the recipe + config metadata are written.Byte-identical
Verified programmatically: for all 11 registered concurrent models, the resolved
prepare_kwargs, the five settings, and the effective slope-priorsigma(plan value, else_default_of) reproduce the former inlinefit_concurrentlogic exactly — including the outcome/predictor de-duplication. End-to-end,ca-001dev-fits clean through all its wave + bivariate sub-fits and now emitsmodel_recipe.md+ a populatedresolved_run_plan(previously absent for the family).14 new run-plan tests (
test_concurrent_run_plan.py); the drift guards and thetest_concurrent_pipelinecharacterisation suite pass;ruff/format:check/spellcheckclean.Scope
Mechanical, behaviour-preserving relocation only — no estimand, likelihood, prior, analysis population, fitted equation, sampling preset, diagnostic threshold or artefact schema change (per #394's contract). Remaining pillar-4 families in the issue's order: mediation (the 3-fit-function outlier — a dedicated multi-PR effort), then aligned/adjusted/growth/historical/measurement.
Assigning to a human for review and merge.